Perf: add support for launch ID#27
Merged
EdmondChuiHW merged 3 commits intofacebook:mainfrom Mar 26, 2024
Merged
Conversation
This was referenced Mar 20, 2024
Closed
Merged
EdmondChuiHW
changed the title
EdmondChuiHW
added a commit
to EdmondChuiHW/react-native
that referenced
this pull request
Mar 21, 2024
Summary: Changelog: [internal] Related PR: facebook/react-native-devtools-frontend#27 Differential Revision: D55164646
2 tasks
EdmondChuiHW
added a commit
to EdmondChuiHW/react-native
that referenced
this pull request
Mar 21, 2024
Summary: Changelog: [internal] Related PR: facebook/react-native-devtools-frontend#27 Differential Revision: D55164646
motiz88
reviewed
Mar 21, 2024
EdmondChuiHW
added a commit
to EdmondChuiHW/react-native
that referenced
this pull request
Mar 21, 2024
Summary: Changelog: [internal] Related PR: facebook/react-native-devtools-frontend#27 Differential Revision: D55164646
huntie
reviewed
Mar 22, 2024
|
|
||
| Host.RNPerfMetrics.registerPerfMetricsGlobalPostMessageHandler(); | ||
|
|
||
| Host.rnPerfMetrics.setLaunchId(Root.Runtime.Runtime.queryParam('launchId')); |
Member
There was a problem hiding this comment.
Different casing here compared to L23? 🤔
Author
There was a problem hiding this comment.
Yeah one is static/constructor. One is the global instance. Following the pattern of the upstream UserMetrics:
Can move the first call to become an instance method instead. At first it was a single function, but this repo enforces a namespace-style import so that's why it was put into RNPerfMetrics "static".
EdmondChuiHW
added a commit
to EdmondChuiHW/react-native
that referenced
this pull request
Mar 28, 2024
Summary: Changelog: [internal] Related PR: facebook/react-native-devtools-frontend#27 Differential Revision: D55164646
EdmondChuiHW
added a commit
to EdmondChuiHW/react-native
that referenced
this pull request
Mar 28, 2024
Summary: Changelog: [internal] Related PR: facebook/react-native-devtools-frontend#27 Reviewed By: hoxyq Differential Revision: D55164646
EdmondChuiHW
added a commit
to EdmondChuiHW/react-native
that referenced
this pull request
Mar 28, 2024
Summary: Changelog: [internal] Related PR: facebook/react-native-devtools-frontend#27 Reviewed By: hoxyq Differential Revision: D55164646
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Mar 28, 2024
Summary: Pull Request resolved: #43585 Changelog: [internal] Related PR: facebook/react-native-devtools-frontend#27 Reviewed By: hoxyq Differential Revision: D55164646 fbshipit-source-id: 0f25f150603a24654020093697e76d85d0d8cc02
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TLDR: We want to keep a stable ID for each launch so we can join CDT sessions.
Open source: no-op as perf metrics is only applicable for internal builds. See #16
Please also see the internal RFC: https://www.internalfb.com/diff/D55164646
This launch ID is read from the query param; thus remains the same for each CDT refresh, either by pressing F5 or clicking "Reconnect DevTools":
https://github.com/facebookexperimental/rn-chrome-devtools-frontend/blob/9370b35e1979ec6953ff59ed78c8751e4f463750/front_end/ui/legacy/RemoteDebuggingTerminatedScreen.ts#L47
Why
Since modern web standards relies on
visibilitychangeto signal the end of a session, we need to account for users showing and hiding the window multiple times. Closing the window is indistinguishable from hiding the window under this model.This means each time the window is hidden, we treat this as one completed CDT session and submit end-of-session perf metrics as if the window is closed.
If the user was only hiding the window and came back later, we need a way to de-dup the subsequent events.
Therefore, we need to keep a stable ID across refreshes and
visibilitychangeevents. For each launch, e.g. from iOS, Android, or Metro, a launch ID is generated in the internal version of Metro. See internal diffWe'll then treat all events with the same launch ID as a single session in our metrics analytics.
Pending further discussion, we can also use this launch ID for Metro and CDP backend to attribute perf metrics outside of CDT.
Stack
Stacked on #26
Depended by #28
Preview this PR in isolation at https://github.com/EdmondChuiHW/rn-chrome-devtools-frontend2/pull/1/files
Test plan
Non-null
launchIdin query param doesn't crash when launched from iOS, Android, and Metro (internal)Null
launchIdquery param: no-op (for open-source)Upstreaming plan
devtools-frontendrepo. I've reviewed the contribution guide.